Fix EHR sql server error #5123
Fix EHR sql server error #5123
Conversation
| @@ -4976,7 +4980,7 @@ else if (scope != tableInfo.getSchema().getScope()) | |||
| } | |||
| commandObject.put("extraContext", commandExtraContext); | |||
|
|
|||
| JSONObject commandResponse = executeJson(commandObject, command, transacted, errors); | |||
| JSONObject commandResponse = executeJson(commandObject, command, !transacted, errors); | |||
There was a problem hiding this comment.
Don't quite get why this got switched to !transacted. This says if it's not transacted we'll allow transaction if the JSON property is true, but if it is transacted here and that property is true, we don't allow transactions?
There was a problem hiding this comment.
It has been like this for 12 years. I think it got flipped because it was trying to avoid nested transaction and have the outer transaction used only. Attempt to reverse is resulted in sql server transaction already rolled back error.
| auditEvent = transaction.getAuditEvent(); | ||
| DbScope.Transaction auditTransaction = transacted ? transaction : table.getSchema().getScope().getCurrentTransaction(); | ||
| if (auditTransaction == null) | ||
| auditTransaction = transaction; |
There was a problem hiding this comment.
I'm not sure I understand this. If transacted is true, we use the ensured transaction from the try block and if it's false we get the transaction from the table schema. Presumably if it's transacted, these are the same thing. If there is no transaction on the table schema, we use the NO_OP_TRANSACTION. So could this be
`
| auditTransaction = transaction; | |
| DbScope.Transaction auditTransaction = table.getSchema().getScope().getCurrentTransaction(); | |
| if (auditTransaction == null) | |
| auditTransaction = NO_OP_TRANSACTION; |
There was a problem hiding this comment.
I've modified the logic a little bit more to only get scope transaction when the outer caller is indeed transacted.
Rationale
#5089 introduced a change to use the same transaction in a nested way. This turns out to be causing issues for sql server, see related failures
This PR reverts the !transacted change to use NO_OP transaction for the inner transaction, and seek to handle transaction audit on an alternative current transaction.
Good run
Related Pull Requests
Changes